-
Notifications
You must be signed in to change notification settings - Fork 25k
feat: Add clip-path support for Android #54701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@jorge-cab has imported this pull request. If you are a Meta employee, you can view this in D88081136. |
jorge-cab
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things here
Right now we are repeating ourselves on every view React Native uses to get clipping correct. I'm hoping there's a better solution. I think we can leverage CompositeBackgroundDrawable
We can try adding a parameter to CompositeBackgroundDrawable's constructor for clipPath. Then let's override draw and clip the canvas there, after which we can call super.draw() don't save/restore, this way the clipping will persist through the entire view's drawing logic since the canvas is shared between the background and the view itself.
This way we avoid the tag, and avoid having to add logic to every single view and also better justify adding the clipping logic to BackgroundStyleApplicator
I'm pretty sure backgrounds are drawn before any of the view's drawing operations so the canvas should remain clipped for any subsequent operations.
I'm really hoping this makes sense since it seems pretty messy to modify every view's drawing methods to get this to work.
@joevilches can you sanity check this?
I might have inaccuracies in my theory here so happy to discuss this a bit more.
Also added some comments that apply regardless of the approach
| import com.facebook.react.uimanager.LengthPercentageType | ||
| import com.facebook.react.uimanager.PixelUtil | ||
|
|
||
| public object ClipPathUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this internal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
| } | ||
|
|
||
| public data class CircleShape( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make all of the added classes internal. We don't want them to be part of the public API without a good reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done - with current solution only ClipPath class is public since it's required by helper function getClipPath in BackgroundStyleApplicator.kt
| @@ -0,0 +1,178 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning behind putting this under react/view/views? This is only used in the uimanager/styles package right? It seems to me like that's a better fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a leftover from the previous versions of code - uimanager/styles is much better place now definitely!
| @ReactProp(name = ViewProps.CLIP_PATH, customType = "ClipPath") | ||
| public override fun setClipPath(view: ReactViewGroup, clipPath: ReadableMap?) { | ||
| if (ViewUtil.getUIManagerType(view) == UIManagerType.FABRIC) { | ||
| BackgroundStyleApplicator.setClipPath(view, clipPath) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding to BaseViewManager is enough, no need to override here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct - dropped this method
| @ReactPropGroup( | ||
| names = | ||
| [ | ||
| ViewProps.MARGIN, | ||
| ViewProps.MARGIN_VERTICAL, | ||
| ViewProps.MARGIN_HORIZONTAL, | ||
| ViewProps.MARGIN_LEFT, | ||
| ViewProps.MARGIN_RIGHT, | ||
| ViewProps.MARGIN_TOP, | ||
| ViewProps.MARGIN_BOTTOM, | ||
| ViewProps.MARGIN_START, | ||
| ViewProps.MARGIN_END | ||
| ], | ||
| defaultFloat = Float.NaN, | ||
| ) | ||
| public open fun setMargin(view: ReactViewGroup, index: Int, margin: Float) { | ||
|
|
||
| val layoutParams = view.layoutParams as? MarginLayoutParams ?: MarginLayoutParams(LayoutParams.WRAP_CONTENT, LayoutParams.WRAP_CONTENT) | ||
| val leftMargin = layoutParams.leftMargin; | ||
| val topMargin = layoutParams.topMargin; | ||
| val rightMargin = layoutParams.rightMargin; | ||
| val bottomMargin = layoutParams.bottomMargin; | ||
| when (MarginIndex.fromIndex(index)) { | ||
| MarginIndex.ALL -> { | ||
| layoutParams.setMargins(margin.toInt(), margin.toInt(), margin.toInt(), margin.toInt()) | ||
| } | ||
| MarginIndex.VERTICAL -> { | ||
| layoutParams.setMargins(leftMargin, margin.toInt(), rightMargin, margin.toInt()) | ||
| } | ||
| MarginIndex.HORIZONTAL -> { | ||
| layoutParams.setMargins(margin.toInt(), topMargin, margin.toInt(), bottomMargin) | ||
| } | ||
| MarginIndex.LEFT -> { | ||
| layoutParams.setMargins(margin.toInt(), topMargin, rightMargin, bottomMargin) | ||
| } | ||
| MarginIndex.RIGHT -> { | ||
| layoutParams.setMargins(leftMargin, topMargin, margin.toInt(), bottomMargin) | ||
| } | ||
| MarginIndex.TOP -> { | ||
| layoutParams.setMargins(leftMargin, margin.toInt(), rightMargin, bottomMargin) | ||
| } | ||
| MarginIndex.BOTTOM -> { | ||
| layoutParams.setMargins(leftMargin, topMargin, rightMargin, margin.toInt()) | ||
| } | ||
| MarginIndex.START -> { | ||
| layoutParams.setMargins(margin.toInt(), topMargin, rightMargin, bottomMargin) | ||
| } | ||
| MarginIndex.END -> { | ||
| layoutParams.setMargins(leftMargin, topMargin, margin.toInt(), bottomMargin) | ||
| } | ||
| null -> { | ||
| // Unknown index, do nothing | ||
| } | ||
| } | ||
| view.layoutParams = layoutParams | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unrelated? Correct me if I'm wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of it is to access margins in the GeometryBoxUtil.kt functions, it's done via view's layoutParams property. Without this population I couldn't get the node's margins, which are required to calculate properly both geometry box bounds and border radius. I couldn't find any other way to obtain these values, I didn't like this solution and would love to replace it and get View margins from some other place.
Instead of storing margins in view's layoutParams, I could store it as Spacing type directly in BackgroundStyleApplicator or in CompositeBackgroundDrawable (if it will be used). It will make this function shorter and perhaps more correct. What do you think about it?
| private enum class MarginIndex { | ||
| ALL, | ||
| VERTICAL, | ||
| HORIZONTAL, | ||
| LEFT, | ||
| RIGHT, | ||
| TOP, | ||
| BOTTOM, | ||
| START, | ||
| END; | ||
|
|
||
| companion object { | ||
| fun fromIndex(index: Int): MarginIndex? = values().getOrNull(index) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it's used to properly set layout's margins, at least to make when block looking a bit nicer.
| public override fun hasOverlappingRendering(): Boolean = false | ||
|
|
||
| public override fun draw(canvas: Canvas) { | ||
| canvas.withSave { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a perf concern. canvas.withSave will do canvas.save and then canvas.restore on every draw operation even when we aren't clipping anything. We should gate this properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced all usages of canvas.withSave to conditional block. I added a getClipPath function to BackgroundStyleApplicator class in order to access the view's tag only there, not in every View class that calls applyClipPathIfPresent() function.
| } | ||
|
|
||
| super.onDraw(canvas) | ||
| canvas.withSave { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, don't run this unconditionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
| public override fun draw(canvas: Canvas) { | ||
| canvas.withSave { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, don't do this unconditionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
| super.onDraw(canvas) | ||
| canvas.withSave { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we sometimes run this code on onDraw and sometimes on draw? We should probably be consistent and just do it on draw every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
d88ccd2 to
7d87540
Compare
7d87540 to
aa37081
Compare
|
Thanks @jorge-cab for the comments and review!
Generally I definitely like your proposed solution much more, but I have tried this approach initially but couldn't get it to work - children were not clipped, only background was. I noticed that I made a second attempt today and ended up with the same result. After the investigation and debugging I noticed that the reason why children may not be clipped is hardware acceleration. In this case drawing of background is isolated and canvas state not shared with the View's drawing (see |
Summary:
This PR adds support for
clip-pathCSS attribute for JS. It follows CSS spec described here. It does not provide support for SVG source (<clip-source>values), what can be added in the additional PR if needed. Supported syntax is[<basic-shape>] || <geometry-box>]with almost full support of every basic shape (<basic-shape> = circle | ellipse | rect | polygon | inset | xywh) and references boxes (<geometry-box> = margin-box | border-box | padding-box | content-box | fill-box | stoke-box | view-box).Work has been split into three PRs for more convenient reviewing process. This part adds Android support for clipping, path and bounds calculations.
Changelog:
[ANDROID] [ADDED] - Add clip-path support for Android
Test Plan:
Merge JS PR and run RNTester app.
ClipPathExample.js)